[Discover] Hide the whole filter div instead of just the icons #11819

weltenwort · 2017-05-16T15:00:16Z

Since #11604 the background of the filter icons in the document table remained visible and covered part of the cell content even when the cell was not hovered. That was caused by moving from display: none on the filter div to opacity: 0 on the individual icons.

This PR moves the background styles from the filter div to the individual icons to prevent it from covering cell content when not hovered.

fixes #12061

cjcenizal

Found an issue, but I think there's an easy solution.

cjcenizal · 2017-05-16T18:38:48Z

src/ui/public/doc_table/doc_table.less

-        .docTableRowFilterButton {
-          opacity: 1;
+        .table-cell-filter {
+          opacity: 1; /* 2 */


Unfortunately, if you tab to the buttons, they won't become visible because these styles only apply when the focus state is on the parent. I think the best solution is to remove the background-color: @panel-bg; property from line 43, and change .docTableRowFilterButton to have these styles:

/** * 1. Align icon with text in cell. * 2. Use opacity to make this element accessible to screen readers and keyboard. * 3. Show on focus to enable keyboard accessibility. */ .docTableRowFilterButton { appearance: none; background-color: @panel-bg; border: none; padding: 0 3px; font-size: 14px; line-height: 1; /* 1 */ display: inline-block; opacity: 0; /* 2 */ &:focus { opacity: 1; /* 3 */ } }

This way the buttons themselves have the background color, so they will only obscure the text when the buttons are focused or the row is being hovered over.

weltenwort · 2017-05-17T09:56:25Z

@cjcenizal thanks for the explanation, fixed it

cjcenizal

Nice! One small detail and then we're good.

cjcenizal · 2017-05-17T18:26:57Z

src/ui/public/doc_table/doc_table.less

@@ -74,7 +73,7 @@ doc-table {
 */
 .docTableRowFilterButton {
  appearance: none;
-  background-color: transparent;
+  background-color: @panel-bg;


I would also add this rule:

padding: 0 3px;

and remove this rule:

& + & { margin-left: 5px; }

This way we can avoid a transparent gap between the buttons.

lukasolson

I merged this PR with your other one so I could see what the filter icons look like. This looks better, but when I hover over a cell in the highlighted row, the background of the icons clashes:

weltenwort · 2017-05-18T09:47:19Z

@lukasolson that's a good point

How about using a text shadow to create a contrast-increasing outline around the icons instead?

@cjcenizal what do you think of that?

cjcenizal · 2017-05-18T16:21:27Z

@weltenwort I like your idea but I think the bits and pieces of the cell text that show around the icons adds too much visual noise. I think we can solve this by updating the rule in discover/styles/main.less on line 135 to be:

    .discover-table-row--highlight {
      td,
      .docTableRowFilterButton {
        background-color: #E2F1F6;
      }
    }

Then the background color of the icons will match the highlighted background color:

weltenwort · 2017-05-19T10:28:53Z

Yes, that technically fixes it, but the coupling without an explicit block definition makes me cringe. It is merely a drop in an ocean of awful css in the doc table though, so here we go.

next up: doc table BEMification

@cjcenizal is there a high chance of conflict refactoring the doc table classes right now or would you say I'm safe to go ahead?

cjcenizal · 2017-05-19T15:58:04Z

makes me cringe

I'm glad I'm not the only one! :)

I think you're safe to refactor the doc table classes. There's nothing in the UI Framework that interacts with it, as far as I know. And I'm not working on any code that will affect it either.

You're a brave man @weltenwort! Viel glück.

weltenwort · 2017-05-31T11:38:24Z

@lukasolson @cjcenizal ready for another look

cjcenizal

LGTM! 🎸

weltenwort · 2017-06-08T10:11:34Z

@lukasolson do you have any further objections?

lukasolson

LGTM!

Backports PR elastic#11819

…12262) Backports PR #11819

…12263) Backports PR #11819

…ic#11819) Since elastic#11604 the background of the filter icons in the document table remained visible and covered part of the cell content even when the cell was not hovered. That was caused by moving from display: none on the filter div to opacity: 0 on the individual icons. This PR moves the background styles from the filter div to the individual icons to prevent it from covering cell content when not hovered.

weltenwort added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. Feature:Discover Discover Application :Discovery v5.5.0 v6.0.0 labels May 16, 2017

weltenwort self-assigned this May 16, 2017

weltenwort requested review from lukasolson and cjcenizal May 16, 2017 15:00

weltenwort mentioned this pull request May 16, 2017

[context view] Apply filters to the context query #11466

Merged

4 tasks

cjcenizal suggested changes May 16, 2017

View reviewed changes

weltenwort force-pushed the fix/doctable-cell-filter-background branch from e199cd2 to 8c15da6 Compare May 17, 2017 08:20

cjcenizal suggested changes May 17, 2017

View reviewed changes

epixa added the v6.0.0-alpha2 label May 17, 2017

lukasolson requested changes May 17, 2017

View reviewed changes

weltenwort force-pushed the fix/doctable-cell-filter-background branch from 8c15da6 to f8c1e68 Compare May 19, 2017 10:28

weltenwort mentioned this pull request May 30, 2017

White label box(?) in log event context #12061

Closed

Move the background to the individual icons

44b3396

weltenwort force-pushed the fix/doctable-cell-filter-background branch from f8c1e68 to 44b3396 Compare May 31, 2017 10:42

cjcenizal approved these changes May 31, 2017

View reviewed changes

cjcenizal mentioned this pull request Jun 1, 2017

Palette swap for accessibility / kibana 6 #12085

Merged

epixa added v6.0.0-alpha3 and removed v6.0.0-alpha2 labels Jun 2, 2017

spalger added the v5.6.0 label Jun 7, 2017

weltenwort added the review label Jun 7, 2017

lukasolson approved these changes Jun 9, 2017

View reviewed changes

weltenwort merged commit 30dcd59 into elastic:master Jun 9, 2017

weltenwort added a commit to weltenwort/kibana that referenced this pull request Jun 9, 2017

[5.x] [Discover] Hide the whole filter div instead of just the icons

fac9217

Backports PR elastic#11819

weltenwort mentioned this pull request Jun 9, 2017

[5.x] [Discover] Hide the whole filter div instead of just the icons #12262

Merged

weltenwort added a commit to weltenwort/kibana that referenced this pull request Jun 9, 2017

[5.5] [Discover] Hide the whole filter div instead of just the icons

800f068

Backports PR elastic#11819

weltenwort mentioned this pull request Jun 9, 2017

[5.5] [Discover] Hide the whole filter div instead of just the icons #12263

Merged

weltenwort added a commit that referenced this pull request Jun 12, 2017

[5.x] [Discover] Hide the whole filter div instead of just the icons (#…

ca2d5ce

…12262) Backports PR #11819

weltenwort added a commit that referenced this pull request Jun 12, 2017

[5.5] [Discover] Hide the whole filter div instead of just the icons (#…

d3edb91

…12263) Backports PR #11819

jimgoodwin added the release_note:fix label Jun 26, 2017

epixa added v6.0.0-beta1 and removed v6.0.0-beta1 v6.0.0-alpha3 labels Jul 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Hide the whole filter div instead of just the icons #11819

[Discover] Hide the whole filter div instead of just the icons #11819

weltenwort commented May 16, 2017 •

edited

Loading

cjcenizal left a comment

cjcenizal May 16, 2017 •

edited

Loading

weltenwort commented May 17, 2017

cjcenizal left a comment

cjcenizal May 17, 2017

lukasolson left a comment

weltenwort commented May 18, 2017

cjcenizal commented May 18, 2017 •

edited

Loading

weltenwort commented May 19, 2017

cjcenizal commented May 19, 2017

weltenwort commented May 31, 2017

cjcenizal left a comment

weltenwort commented Jun 8, 2017

lukasolson left a comment

[Discover] Hide the whole filter div instead of just the icons #11819

[Discover] Hide the whole filter div instead of just the icons #11819

Conversation

weltenwort commented May 16, 2017 • edited Loading

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal May 16, 2017 • edited Loading

Choose a reason for hiding this comment

weltenwort commented May 17, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal May 17, 2017

Choose a reason for hiding this comment

lukasolson left a comment

Choose a reason for hiding this comment

weltenwort commented May 18, 2017

cjcenizal commented May 18, 2017 • edited Loading

weltenwort commented May 19, 2017

cjcenizal commented May 19, 2017

weltenwort commented May 31, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

weltenwort commented Jun 8, 2017

lukasolson left a comment

Choose a reason for hiding this comment

weltenwort commented May 16, 2017 •

edited

Loading

cjcenizal May 16, 2017 •

edited

Loading

cjcenizal commented May 18, 2017 •

edited

Loading